feat(parquet): wire in additional parquet writer settings by xanderbailey · Pull Request #2887 · apache/iceberg-rust

xanderbailey · 2026-07-24T12:42:04Z

Which issue does this PR close?

Closes Support Iceberg table properties #2659

What changes are included in this PR?

ParquetWriterBuilder::from_table_properties previously only translated the content-defined-chunking settings; every other Parquet knob silently fell through to parquet-rs (arrow-rs) defaults, which diverge from the Java implementation. This PR maps the row-group / page / dictionary sizing and compression table properties into the parquet-rs WriterProperties:

write.parquet.compression-codec (default zstd) and write.parquet.compression-level
write.parquet.row-group-size-bytes (default 128 MB)
write.parquet.page-size-bytes (default 1 MB)
write.parquet.page-row-limit (default 20000)
write.parquet.dict-size-bytes (default 2 MB)

How the new Iceberg defaults differ from the parquet-rs defaults that were previously in effect:

compression: parquet-rs defaults to UNCOMPRESSED, so files were previously written uncompressed; the Java default is zstd.
row group size: parquet-rs sets no byte limit and flushes row groups by row count only (~1M rows); we now apply a 128 MB byte budget. The row-count limit is left at the parquet-rs default since Iceberg has no property for it, so row groups now flush at 128 MB or ~1M rows, whichever comes first.
dictionary page size: parquet-rs defaults to 1 MB; the Iceberg default is 2 MB.
page size (1 MB) and page row limit (20000) already matched parquet-rs, so those are unchanged in practice.

Are these changes tested?

Yes

xanderbailey · 2026-07-24T12:45:45Z

+    /// writer is built, not when properties are parsed.
+    pub const PROPERTY_PARQUET_COMPRESSION_CODEC: &str = "write.parquet.compression-codec";
+    /// Default Parquet compression codec (matches Iceberg's default since 1.4.0).
+    pub const PROPERTY_PARQUET_COMPRESSION_CODEC_DEFAULT: &str = "zstd";


Have called this out in the PR description but current default is actually uncompressed from what I can see.

This opens up a question generally as to if we should copy the Java defaults for these settings which might be more intuitive for user or keep the defaults as the arrow-rs defaults... I don't have a strong opinion here so interested to hear people's thoughts.

On the matter of inconsistency, there are other places where arrow-rs has different defaults to parquet-java, like the compression level for zstd too:

Defaults to 1 for Rust

Defaults to 3 for Java

I'm aligned with Matt's "Principle of Least Astonishment" and with the outcome from the Rust sync - let's adopt Java defaults unless we have a reason to diverge.

xanderbailey · 2026-07-24T12:46:39Z

        assert_eq!(upper_bounds, HashMap::from([(0, Datum::int(i32::MAX))]));
    }

-    // -----------------------------------------------------------------


This looked very claude so I removed

xanderbailey · 2026-07-26T20:15:27Z

Think I'll bring this one up as a topic for the rust community sync. It's not actually clear to me what our defaults should be here and if we should change them. I can see arguments both ways.

mbutrovich · 2026-07-27T13:05:07Z

My suggestion is to follow the Principle of Least Astonishment. Users interacting with their data lake across different Iceberg implementations might be surprised to get wildly different file size results from the de facto reference implementation (Java). I think in this case adhering to Java defaults, since it's likely something I don't expect to be codified in the spec, is reasonable.

xanderbailey · 2026-07-27T13:06:38Z

I'm inclined to agree with this take @mbutrovich

Kurtiscwright · 2026-07-30T15:00:40Z

+        "lz4" => Compression::LZ4,
+        "lz4_raw" => Compression::LZ4_RAW,
+        "gzip" => {
+            let level = match level {


Can we please have these compression levels default to the same values as Java if a user doesn't pass us a compression level. Please reference this comment for examples - https://github.com/apache/iceberg-rust/pull/2887/changes#r3646169572

Kurtiscwright · 2026-07-30T15:04:19Z

Overall looks great, thank you for finding and fixing this! I wonder if we could you use the comet testing proposal to validate the physical parquet matches between Java and Rust.

xanderbailey · 2026-07-30T23:51:05Z

+const DEFAULT_ZSTD_COMPRESSION_LEVEL: i32 = 3;
+/// Default gzip level.
+const DEFAULT_GZIP_COMPRESSION_LEVEL: u32 = 6;
+/// Default brotli level.
+const DEFAULT_BROTLI_COMPRESSION_LEVEL: u32 = 1;


Note that I'm not using the compression codec struct here because ti doesn't have all parquet compression types enumerated. That struct is used for metadata / avro compression levels etc rather than data files and I didn't want to leak parquet into the core crate.

I think we should use the types in the compression module, unless we specifically want to diverge. While they're used primarily for metadata and puffin right now, I think they were written with data files in mind.

I'd extend that module to support brotli. It'll give us a common module where the conversions and errors are tested.

Kurtiscwright

Looks just about ready to merge. Left 2 new comments, one is a request to santa claus and the other is a code style nit-pick so nothing that I would argue should block a merge.

Kurtiscwright · 2026-07-31T05:25:18Z

+            Compression::BROTLI(level)
+        }
+        "zstd" => {
+            let level = level.unwrap_or(DEFAULT_ZSTD_COMPRESSION_LEVEL);


Is there a particular reason to move away from the match level {} pattern and the level_as_u32?

It’s actually just an parquet-rs api limitation, zstd accepts i32, the others require u32

Kurtiscwright · 2026-07-31T05:26:21Z

 use crate::writer::{CurrentFileStatus, DataFile};
 use crate::{Error, ErrorKind, Result};

+/// Default compression levels, pinned to parquet-java's defaults so files are


I wish there was an integration test that would start failing if parquet-java ever changed their defaults.

I don’t think we have any Java tests in the repo so I’m not sure the lift of adding those is worth it for this PR, what do you think?

I don't think there's anything for this PR.

That being said, the proposed shared testing library might be interesting. I wonder if that can capture "this is what a Iceberg data file in Parquet looks like with table properties [X, Y, Z]". Then we can assert encodings, etc..

I think its just something to keep in mind for the future, at this point.

dannycjones

Thanks @xanderbailey, this has been on my TODO list, great to see these wired up!

My primary concern at the moment is around adding behavior beyond Iceberg's Java implementation.

dannycjones · 2026-07-31T08:09:01Z

+    /// Parquet compression codec name (e.g. `zstd`, `gzip`). Validated when the
+    /// writer is built, not when properties are parsed.
+    pub parquet_compression_codec: String,


Validated when the writer is built, not when properties are parsed.

This is a good tenet for the whole module - i.e. we push validation to access, since we may not care if we're writing ORC files (as an example).

If you end up pushing another commit, I suggest we add it to the module comment.

dannycjones · 2026-07-31T08:28:42Z

+    /// writer is built, not when properties are parsed.
+    pub const PROPERTY_PARQUET_COMPRESSION_CODEC: &str = "write.parquet.compression-codec";
+    /// Default Parquet compression codec (matches Iceberg's default since 1.4.0).
+    pub const PROPERTY_PARQUET_COMPRESSION_CODEC_DEFAULT: &str = "zstd";


I'm aligned with Matt's "Principle of Least Astonishment" and with the outcome from the Rust sync - let's adopt Java defaults unless we have a reason to diverge.

dannycjones · 2026-07-31T10:36:53Z

 use crate::writer::{CurrentFileStatus, DataFile};
 use crate::{Error, ErrorKind, Result};

+/// Default compression levels, pinned to parquet-java's defaults so files are


I don't think there's anything for this PR.

That being said, the proposed shared testing library might be interesting. I wonder if that can capture "this is what a Iceberg data file in Parquet looks like with table properties [X, Y, Z]". Then we can assert encodings, etc..

I think its just something to keep in mind for the future, at this point.

dannycjones · 2026-07-31T10:38:08Z

+/// Default compression levels, pinned to parquet-java's defaults so files are
+/// comparable across implementations rather than tracking the parquet-rs
+/// defaults, which may differ and could change without us noticing.


should this be a standard comment or module comment? this is currently attached to DEFAULT_ZSTD_COMPRESSION_LEVEL.

dannycjones · 2026-07-31T10:40:32Z

    ///
-    /// Currently translates the content-defined-chunking keys
-    /// (`write.parquet.content-defined-chunking.*`); other keys fall back to
    /// parquet-rs defaults.


dead comment

dannycjones · 2026-07-31T12:06:40Z

+const DEFAULT_ZSTD_COMPRESSION_LEVEL: i32 = 3;
+/// Default gzip level.
+const DEFAULT_GZIP_COMPRESSION_LEVEL: u32 = 6;
+/// Default brotli level.
+const DEFAULT_BROTLI_COMPRESSION_LEVEL: u32 = 1;


I think we should use the types in the compression module, unless we specifically want to diverge. While they're used primarily for metadata and puffin right now, I think they were written with data files in mind.

I'd extend that module to support brotli. It'll give us a common module where the conversions and errors are tested.

dannycjones · 2026-07-31T12:15:09Z

+        "snappy" => Compression::SNAPPY,
+        "lzo" => Compression::LZO,
+        "lz4" => Compression::LZ4,
+        "lz4_raw" => Compression::LZ4_RAW,


Where does this come from? I don't see it in the iceberg-java repo, and it's not documented for iceberg-java: https://iceberg.apache.org/docs/latest/configuration/#write-properties

I'd rather we don't go beyond Java without intent.

Same for lzo.

I don’t think this is out of spec however, I would claim if a user wants to compress with a format supported by the parquet spec, this should be allowed?

It's not about the spec, since I don't think the writer properties are considered part of the spec broadly.

I'm just a bit defensive of adding logic beyond what Java supports, since it is broadening the feature surface we're responsible for. Especially with Parquet, where some of the changes introduced are not forwards compatible.

I'm not strongly against adding these, happy to hear thoughts from others on this.

dannycjones · 2026-07-31T12:35:54Z

With this merged, I don't think we should close #2659 without a proper review into all configurations offered by the Java implementation. The scope is more than just Parquet writer properties.

Happy to scope that down, but I'd like to have an issue somewhere to track the overall compatibility with table properties respected by Java.

feat(paquet): wire in additional parquet writer settings

ca7d5ba

xanderbailey force-pushed the xb/parquet_writer_settings branch from abcfba9 to ca7d5ba Compare July 24, 2026 12:44

xanderbailey commented Jul 24, 2026

View reviewed changes

xanderbailey changed the title ~~feat(paquet): wire in additional parquet writer settings~~ feat(parquet): wire in additional parquet writer settings Jul 24, 2026

mbutrovich self-requested a review July 24, 2026 17:15

Kurtiscwright suggested changes Jul 30, 2026

View reviewed changes

xanderbailey commented Jul 30, 2026

View reviewed changes

xanderbailey force-pushed the xb/parquet_writer_settings branch from bc85d99 to bc86d9f Compare July 31, 2026 00:26

xanderbailey added 2 commits July 31, 2026 01:27

fix compression level

67c28d8

fmt

da6cb46

xanderbailey force-pushed the xb/parquet_writer_settings branch from bc86d9f to da6cb46 Compare July 31, 2026 00:27

Kurtiscwright reviewed Jul 31, 2026

View reviewed changes

dannycjones reviewed Jul 31, 2026

View reviewed changes

Uh oh!

Conversation

xanderbailey commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xanderbailey commented Jul 26, 2026

Uh oh!

mbutrovich commented Jul 27, 2026

Uh oh!

xanderbailey commented Jul 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kurtiscwright commented Jul 30, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kurtiscwright left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannycjones left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannycjones commented Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xanderbailey commented Jul 24, 2026 •

edited

Loading